-
Notifications
You must be signed in to change notification settings - Fork 121
Feature/slac api #1570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/slac api #1570
Conversation
* Added slac_API module - External API for SLAC * Added BUSlac module - BringUp module for SLAC * Added API documentation Signed-off-by: Piet Gömpel <[email protected]>
Signed-off-by: Piet Gömpel <[email protected]>
|
@djchhp all changes of the original PR had been adressed. |
djchhp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
| }); | ||
|
|
||
| screen.Loop(main_renderer); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it documented somewhere that ready() doesn't need to return?
Other than in the bringup modules I've not seen this pattern. My concern is that it is making an assumption about how the framework works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik it's not documented. The documentation of the framework including startup behavior, module communication, cmd/var handling, etc. should in general be improved. I created an issue for this: #1613
Describe your changes
Added support for external slac API module:
Issue ticket number and link
Original PR: #1463
Checklist before requesting a review